Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove unused CentOS file for x86_64. #1133

Merged
merged 1 commit into from
Nov 14, 2022
Merged

Remove unused CentOS file for x86_64. #1133

merged 1 commit into from
Nov 14, 2022

Conversation

Alexhuszagh
Copy link
Contributor

No description provided.

@Alexhuszagh Alexhuszagh added the no changelog A valid PR without changelog (no-changelog) label Nov 13, 2022
@Alexhuszagh Alexhuszagh requested a review from a team as a code owner November 13, 2022 18:42
@Alexhuszagh
Copy link
Contributor Author

This file currently isn't used, but it probably should be when we have much more native support for ARM64 hosts. I really don't want to implement this currently, but making it clear this file isn't used it a good idea.

Also, how our process for detecting native Dockerfiles works is that we first locate the non-native Dockerfile, and error if it isn't found. This would cause an issue if we remove this entirely.

bors try --target x86_64-unknown-linux-gnu.centos

@Alexhuszagh Alexhuszagh mentioned this pull request Nov 13, 2022
Merged
bors bot added a commit that referenced this pull request Nov 13, 2022
@bors
Try #1133: --target x86_64-unknown-linux-gnu.centos
d5e82b3
@bors
Copy link
Contributor

bors bot commented Nov 13, 2022

try

Build succeeded:

@Emilgardis
Copy link
Member

I say no to this, I don't see the need since

❯ cargo build-docker-image --platform aarch64-unknown-linux-gnu x86_64-unknown-linux-gnu 
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `target/debug/xtask build-docker-image --platform aarch64-unknown-linux-gnu x86_64-unknown-linux-gnu`
[cross] note: Build x86_64-unknown-linux-gnu for aarch64-unknown-linux-gnu
[+] Building 357.2s (23/23) FINISHED                                                                                                                                                                                  
 => [internal] load build definition from Dockerfile.x86_64-unknown-linux-gnu                                                                                                                                    0.0s
 => => transferring dockerfile: 1.17kB                                                                                                                                                                           0.0s
 => [internal] load .dockerignore                                                                                                                                                                                0.0s
 => => transferring context: 223B                                                                                                                                                                                0.0s
 => [internal] load metadata for docker.io/library/ubuntu:20.04                                                                                                                                                  2.4s
 => importing cache manifest from ghcr.io/cross-rs/x86_64-unknown-linux-gnu:main                                                                                                                                 0.0s
 => [internal] load build context                                                                                                                                                                                0.0s
 => => transferring context: 32.38kB                                                                                                                                                                             0.0s
 => [ 1/17] FROM docker.io/library/ubuntu:20.04@sha256:450e066588f42ebe1551f3b1a535034b6aa46cd936fe7f2c6b0d72997ec61dbd                                                                                          2.2s
 => => resolve docker.io/library/ubuntu:20.04@sha256:450e066588f42ebe1551f3b1a535034b6aa46cd936fe7f2c6b0d72997ec61dbd                                                                                            0.0s
 => => sha256:450e066588f42ebe1551f3b1a535034b6aa46cd936fe7f2c6b0d72997ec61dbd 1.42kB / 1.42kB                                                                                                                   0.0s
 => => sha256:72dd010fadf5d9da8197f433fa6ac50179307ce0fc5ac690651518ffa7473885 529B / 529B                                                                                                                       0.0s
 => => sha256:89867091bfb2626393d967e92b55842b547f249ec12469488e0eb5ac1f3c1ae4 1.48kB / 1.48kB                                                                                                                   0.0s
 => => sha256:4e7e0215f4adc2c48ad9cb3b3781e21d474b477587f85682c2e2975ae91dce9d 27.20MB / 27.20MB                                                                                                                 1.3s
 => => extracting sha256:4e7e0215f4adc2c48ad9cb3b3781e21d474b477587f85682c2e2975ae91dce9d                                                                                                                        0.7s
 => [ 2/17] COPY common.sh lib.sh /                                                                                                                                                                              0.1s
 => [ 3/17] RUN /common.sh                                                                                                                                                                                      46.9s
 => [ 4/17] COPY cmake.sh /                                                                                                                                                                                      0.1s 
 => [ 5/17] RUN /cmake.sh                                                                                                                                                                                        7.7s 
 => [ 6/17] COPY xargo.sh /                                                                                                                                                                                      0.0s
 => [ 7/17] RUN /xargo.sh                                                                                                                                                                                       77.2s
 => [ 8/17] RUN apt-get update && apt-get install --assume-yes --no-install-recommends     g++-x86-64-linux-gnu     libc6-dev-amd64-cross                                                                        8.5s
 => [ 9/17] COPY deny-debian-packages.sh /                                                                                                                                                                       0.0s
 => [10/17] RUN TARGET_ARCH=amd64 /deny-debian-packages.sh     binutils     binutils-x86-64-linux-gnu                                                                                                            0.2s
 => [11/17] COPY qemu.sh /                                                                                                                                                                                       0.0s
 => [12/17] RUN /qemu.sh x86_64 softmmu                                                                                                                                                                        157.1s
 => [13/17] COPY dropbear.sh /                                                                                                                                                                                   0.0s
 => [14/17] RUN /dropbear.sh                                                                                                                                                                                    19.5s
 => [15/17] COPY linux-image.sh /                                                                                                                                                                                0.0s
 => [16/17] RUN /linux-image.sh x86_64                                                                                                                                                                          33.0s
 => [17/17] COPY linux-runner base-runner.sh /                                                                                                                                                                   0.0s
 => exporting to image                                                                                                                                                                                           2.0s
 => => exporting layers                                                                                                                                                                                          2.0s
 => => writing image sha256:6a1674b0128506550742de900001125113aa69ad5ec07fe6d78e2453061a7c59                                                                                                                     0.0s
 => => naming to ghcr.io/cross-rs/x86_64-unknown-linux-gnu:local

works fine

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Nov 13, 2022 •
edited
Loading

I failed to communicate the intent and rationale of this:

  1. We need this file to exist and to have the FROM ubuntu:20.04 lines for our build system, since that's how we process native versus non-native Dockerfiles. Otherwise, building a docker image will fail due to being unable to find this Dockerfile.
  2. This file can never be used, since it can only be built on an x86_64 host due to assumptions about the compiler/etc., but we already use the Dockerfile.native.centos for that.
  3. To implement it for a non-x86_64 host, we'd need something a script to install the entire toolchain like aarch64-linux-gnu-glibc.sh, which I personally am not planning to implement/do anytime soon.
  4. It effectively duplicates Dockerfile.native.centos, with just a few hard-coded assumptions, and eventually will become irrelevant.

Marking it as unused, without actually having it do anything, is probably the best solution: I'd rather not add additional logic to our build system just for this file. Dockerfile.x86_64-unknown-linux-gnu is meant for a non-x86_64 host, and that's why it works. Whether or not we need support for an older glibc for this target is irrelevant here right now (it's at best fairly low priority for me): the contents of this file simply are not used.

@Emilgardis
Copy link
Member

ofc, I built the wrong image...

anyway, I'd prefer it if we keep the file. Instead I think we can move it to docker/unused/ in one commit, and create a new file in another (to keep line history)

Does that seem fair?

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Nov 14, 2022 •
edited
Loading

We can, I just think the file is practically identical to Dockerfile.native.centos, which just changes x86_64 hard-coded to be abstract for any architecture. The differences are:

4,7d3
< ARG TARGETARCH
< ARG TARGETVARIANT
< ARG CROSS_TARGET_TRIPLE
< 
9,10c5,6
< COPY linux-image.sh native-linux-image.sh /
< RUN /native-linux-image.sh
---
> COPY linux-image.sh /
> RUN /linux-image.sh x86_64
23,29c19,20
< # these need to be present in **both** FROM sections
< ARG TARGETARCH
< ARG TARGETVARIANT
< ARG CROSS_TARGET_TRIPLE
< 
< COPY qemu.sh native-qemu.sh /
< RUN /native-qemu.sh
---
> COPY qemu.sh /
> RUN /qemu.sh x86_64 softmmu
36c27
< COPY linux-runner native-linux-runner base-runner.sh /
---
> COPY linux-runner base-runner.sh /
38,39d28
< ENV CROSS_TARGETARCH=$TARGETARCH
< ENV CROSS_TARGETVARIANT=$TARGETVARIANT
42c31
< ENV CARGO_TARGET_${CROSS_TARGET_TRIPLE}_RUNNER="/native-linux-runner"
---
> ENV CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER="/linux-runner x86_64

Or, linux-image.sh becomes native-linux-image.sh, qemu.sh becomes native-qemu.sh, and linux-runner becomes native-linux-runner. Since the file isn't deleted, its history will stay as well. But I'm fine either way.

Emilgardis
Emilgardis approved these changes Nov 14, 2022
View reviewed changes
Copy link
Member

@Emilgardis Emilgardis left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after some contemplation, I think this is fine now :)

the image works with cargo build-docker-image --platform aarch64-unknown-linux-gnu x86_64-unknown-linux-gnu.centos but is pretty much unusable

@Alexhuszagh
Copy link
Contributor Author

bors r=Emilgardis

bors bot added a commit that referenced this pull request Nov 14, 2022
@bors @Alexhuszagh
Merge #1133
8108f90 
1133: Remove unused CentOS file for x86_64. r=Emilgardis a=Alexhuszagh



Co-authored-by: Alex Huszagh <[email protected]>
@bors
Copy link
Contributor

bors bot commented Nov 14, 2022

Build failed:

@Alexhuszagh
Copy link
Contributor Author

Looks like GNU Savannah's temporarily down: will retry later once it's back up.

@Alexhuszagh
Copy link
Contributor Author

bors retry

@bors
Copy link
Contributor

bors bot commented Nov 14, 2022

Build succeeded:

@bors bors bot merged commit 41b63f7 into cross-rs:main Nov 14, 2022
@Alexhuszagh Alexhuszagh deleted the no_centos branch November 14, 2022 02:06
@Emilgardis Emilgardis added this to the v0.3.0 milestone Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Emilgardis Emilgardis Emilgardis approved these changes

Assignees
No one assigned
Labels
no changelog A valid PR without changelog (no-changelog)
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Alexhuszagh @Emilgardis